Skip to content

fix: prevent self-kill and ensure graceful shutdown with terminal flush - #886

Open
ilyaxuwu wants to merge 1 commit into
CodebuffAI:mainfrom
ilyaxuwu:fix/self-kill-guard-graceful-shutdown
Open

fix: prevent self-kill and ensure graceful shutdown with terminal flush#886
ilyaxuwu wants to merge 1 commit into
CodebuffAI:mainfrom
ilyaxuwu:fix/self-kill-guard-graceful-shutdown

Conversation

@ilyaxuwu

Copy link
Copy Markdown

This should works now! https://streamable.com/r42l5x If it doesn't work I would give up

@jahooma

jahooma commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

I'll have my agents take a look

@codebuff-team

Copy link
Copy Markdown
Contributor

Thanks for digging into the exit-cleanup path — the underlying idea (make sure buffered terminal escape sequences reach the tty before the process dies) is legitimate and worth pursuing. A few concerns before this is portable, though:

  1. command-registry.ts: replacing process.kill(process.pid, 'SIGINT') with process.exit(0) changes behavior, not just timing. The SIGINT handler (via use-exit-handler) presumably drives the async cleanup path in freebuff-exit.ts (endFreebuffSessionBestEffort, flushAnalytics, stopActiveRun). process.exit(0) only fires the synchronous 'exit' listeners, so the exit/quit/q command will now skip session-end and analytics flushing entirely. That's a regression, not a fix, unless those calls are also wired into the 'exit' handler.

  2. renderer-cleanup.ts: process.stdout._handle?.setBlocking?.(true) reaches into a private, untyped Node internal (_handle isn't on the WriteStream type), which will likely fail TypeScript compilation without an any cast and is fragile across Node/Bun versions. Also, reusing handlersInstalled (previously an installation guard) to also guard against double-cleanup() calls conflates two unrelated concerns and could silently disable handler re-installation elsewhere.

  3. run-terminal-command.ts: the child.pid === process.pid guards in killProcessGroup/exit sweep protect against a scenario (pid collision between parent and its own child) that essentially can't happen under normal OS pid allocation. This reads as defensive code added without a concrete repro, which raises the question of whether the actual root cause of the terminal garbling was diagnosed.

The freebuff-exit.ts flush addition is the most clearly beneficial and low-risk piece here. I'd suggest splitting that out, dropping the self-kill guards unless you have a reproducible case, and reverting the exit command to keep the async cleanup path intact (or explicitly wiring cleanup into process.on('exit') if you want the synchronous route).

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants